Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Push ast::{ItemKind, ImplItemKind}::OpaqueTy hack down into lowering #66197

Merged
merged 9 commits into from
Nov 15, 2019

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Nov 7, 2019

We currently have a hack in the form of ast::{ItemKind, ImplItemKind}::OpaqueTy which is constructed literally when you write type Alias = impl Trait; but not e.g. type Alias = Vec<impl Trait>;. Per rust-lang/rfcs#2515, this needs to change to allow impl Trait in nested positions. This PR achieves this change for the syntactic aspect but not the semantic one, which will require changes in lowering and def collection. In the interim, TyKind::opaque_top_hack is introduced to avoid knock-on changes in lowering, collection, and resolve. These hacks can then be removed and fixed one by one until the desired semantics are supported.

r? @varkor

@Centril Centril added the F-type_alias_impl_trait `#[feature(type_alias_impl_trait)]` label Nov 7, 2019
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 7, 2019
@bors

This comment has been minimized.

ItemKind::TyAlias(ref ty, _) => {
let def_kind = match ty.kind.opaque_top_hack() {
None => DefKind::TyAlias,
Some(_) => DefKind::OpaqueTy,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, I noticed today that DefKind::OpaqueTy and DefKind::AssocOpaqueTy exist despite the existential type syntax being removed.

Is it possible to remove them after this PR or it will require reworking lowering as well?

Copy link
Contributor Author

@Centril Centril Nov 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure. We might need something like:

type Foo = (impl Bar, impl Baz); // In AST

=> (HIR)

opaque type _0: Bar; // DefKind::OpaqueTy
opaque type _1: Baz; // DefKind::OpaqueTy
type Foo = ( // DefKind::TyAlias
     _0, // TyKind::Def(..)
     _1 // TyKind::Def(..)
);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my point of view: as long we still have something like opaque type in HIR still, that's good enough for my upcoming PR. I'd rather not remove that for a while yet.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We won't get rid of opaque types in HIR (they will always just arise from type aliases).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's what I thought. :-)

@bors

This comment has been minimized.

@varkor
Copy link
Member

varkor commented Nov 11, 2019

Looks good to me, thanks!

@bors r+

@bors
Copy link
Contributor

bors commented Nov 11, 2019

📌 Commit c43e4d1 has been approved by varkor

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 11, 2019
@bors
Copy link
Contributor

bors commented Nov 13, 2019

⌛ Testing commit c43e4d1 with merge a2ae41c...

bors added a commit that referenced this pull request Nov 13, 2019
Push `ast::{ItemKind, ImplItemKind}::OpaqueTy` hack down into lowering

We currently have a hack in the form of `ast::{ItemKind, ImplItemKind}::OpaqueTy` which is constructed literally when you write `type Alias = impl Trait;` but not e.g. `type Alias = Vec<impl Trait>;`. Per rust-lang/rfcs#2515, this needs to change to allow `impl Trait` in nested positions.  This PR achieves this change for the syntactic aspect but not the semantic one, which will require changes in lowering and def collection. In the interim, `TyKind::opaque_top_hack` is introduced to avoid knock-on changes in lowering, collection, and resolve. These hacks can then be removed and fixed one by one until the desired semantics are supported.

r? @varkor
@rust-highfive

This comment has been minimized.

@bors

This comment has been minimized.

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 13, 2019
@Centril
Copy link
Contributor Author

Centril commented Nov 14, 2019

Ran ./x.py -i test src/test/ui --stage 1 --bless --pass check --compare-mode=nll

@bors r=varkor

@bors
Copy link
Contributor

bors commented Nov 14, 2019

📌 Commit 03cf0d7 has been approved by varkor

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 14, 2019
tmandry added a commit to tmandry/rust that referenced this pull request Nov 14, 2019
Push `ast::{ItemKind, ImplItemKind}::OpaqueTy` hack down into lowering

We currently have a hack in the form of `ast::{ItemKind, ImplItemKind}::OpaqueTy` which is constructed literally when you write `type Alias = impl Trait;` but not e.g. `type Alias = Vec<impl Trait>;`. Per rust-lang/rfcs#2515, this needs to change to allow `impl Trait` in nested positions.  This PR achieves this change for the syntactic aspect but not the semantic one, which will require changes in lowering and def collection. In the interim, `TyKind::opaque_top_hack` is introduced to avoid knock-on changes in lowering, collection, and resolve. These hacks can then be removed and fixed one by one until the desired semantics are supported.

r? @varkor
tmandry added a commit to tmandry/rust that referenced this pull request Nov 14, 2019
Push `ast::{ItemKind, ImplItemKind}::OpaqueTy` hack down into lowering

We currently have a hack in the form of `ast::{ItemKind, ImplItemKind}::OpaqueTy` which is constructed literally when you write `type Alias = impl Trait;` but not e.g. `type Alias = Vec<impl Trait>;`. Per rust-lang/rfcs#2515, this needs to change to allow `impl Trait` in nested positions.  This PR achieves this change for the syntactic aspect but not the semantic one, which will require changes in lowering and def collection. In the interim, `TyKind::opaque_top_hack` is introduced to avoid knock-on changes in lowering, collection, and resolve. These hacks can then be removed and fixed one by one until the desired semantics are supported.

r? @varkor
@bors
Copy link
Contributor

bors commented Nov 15, 2019

⌛ Testing commit 03cf0d7 with merge 29095e1395f6f446330750531b65536d9066d6f7...

Centril added a commit to Centril/rust that referenced this pull request Nov 15, 2019
Push `ast::{ItemKind, ImplItemKind}::OpaqueTy` hack down into lowering

We currently have a hack in the form of `ast::{ItemKind, ImplItemKind}::OpaqueTy` which is constructed literally when you write `type Alias = impl Trait;` but not e.g. `type Alias = Vec<impl Trait>;`. Per rust-lang/rfcs#2515, this needs to change to allow `impl Trait` in nested positions.  This PR achieves this change for the syntactic aspect but not the semantic one, which will require changes in lowering and def collection. In the interim, `TyKind::opaque_top_hack` is introduced to avoid knock-on changes in lowering, collection, and resolve. These hacks can then be removed and fixed one by one until the desired semantics are supported.

r? @varkor
@Centril
Copy link
Contributor Author

Centril commented Nov 15, 2019

@bors retry rolled up.

tmandry added a commit to tmandry/rust that referenced this pull request Nov 15, 2019
Push `ast::{ItemKind, ImplItemKind}::OpaqueTy` hack down into lowering

We currently have a hack in the form of `ast::{ItemKind, ImplItemKind}::OpaqueTy` which is constructed literally when you write `type Alias = impl Trait;` but not e.g. `type Alias = Vec<impl Trait>;`. Per rust-lang/rfcs#2515, this needs to change to allow `impl Trait` in nested positions.  This PR achieves this change for the syntactic aspect but not the semantic one, which will require changes in lowering and def collection. In the interim, `TyKind::opaque_top_hack` is introduced to avoid knock-on changes in lowering, collection, and resolve. These hacks can then be removed and fixed one by one until the desired semantics are supported.

r? @varkor
bors added a commit that referenced this pull request Nov 15, 2019
Rollup of 4 pull requests

Successful merges:

 - #66197 (Push `ast::{ItemKind, ImplItemKind}::OpaqueTy` hack down into lowering)
 - #66429 (Add a regression test for #62524)
 - #66435 (Correct `const_in_array_repeat_expressions` feature name)
 - #66443 (Port erased cleanup)

Failed merges:

r? @ghost
@bors
Copy link
Contributor

bors commented Nov 15, 2019

⌛ Testing commit 03cf0d7 with merge 1bd30ce...

@bors bors merged commit 03cf0d7 into rust-lang:master Nov 15, 2019
@Centril Centril deleted the transparent-ast branch November 15, 2019 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-type_alias_impl_trait `#[feature(type_alias_impl_trait)]` S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Development

Successfully merging this pull request may close these issues.

6 participants